-
Notifications
You must be signed in to change notification settings - Fork 485
Add WithXx overloads that take target instance #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also test that adding a static method using these overloads works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the test failures and it appears that this new WithTools<TToolType>(TToolType target, ...)
overload is now being preferred over the WithTools(IEnumerable<McpServerTool> tools)
if the compile-time type of the tools being passed is something like McpServerTool[]
or List<McpServerTool>
instead of exactly IEnumerable<McpServerTool>
.
Ouch. Not sure why I didn't see those failures when I ran tests locally, but yeah, that's an issue. We can't constraint the TToolType, because any type could legitimately have attributed methods on it. Seems like we probably just want to give it a different name. |
A different name for is probably the best bet to avoid the ambiguity, but I'm not sure what we'd call it. Maybe I wonder if we could raise an error earlier in cases like these, and throw immediately if I don't think this would make sense for when Assembly or Type objects are passed as runtime parameters, since you might be programmatically adding a bunch of stuff you're not sure will have any handlers, but it would be very strange to register a type using compile-time generic type parameters without any handlers. It's too bad we cannot make this a compile-time error, but I think a runtime error is probably better than nothing. |
How would it be to do an |
I went with this suggestion. Initially it felt wrong to me, but then I started thinking about it as a feature, where the TType gets to be a provider of tools if it chooses to implement that interface. Plus, an arbitrary type really wouldn't have any reason to implement that interface unless it wanted this behavior, I think. So, let's try it. I didn't address any of the other argument validation questions. We can address that separately if desired. |
Closes #697
cc: @asklar